-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[dotnet] [bidi] Modules as extensions #16392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
@RenderMichael despite on these dummy changes, let's see how we can modify If you know please advise. |
PR Code Suggestions ✨Explore these optional code suggestions:
|
I don't want to believe that injecting custom |
dotnet/src/webdriver/BiDi/Module.cs
Outdated
protected Broker Broker { get; } = broker; | ||
protected Broker Broker { get; private set; } | ||
|
||
protected internal abstract void Initialize(Broker broker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want third-party code to be able to make their own modules? If so, we can make this protected
instead of protected internal
so others can override.
|
||
var data = JsonSerializer.SerializeToUtf8Bytes(command, typeof(TCommand), _jsonSerializerContext); | ||
|
||
var data = JsonSerializer.SerializeToUtf8Bytes(command, typeof(TCommand), _jsonSerializerOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we are passing in options instead of context, we can make this strongly typed
var data = JsonSerializer.SerializeToUtf8Bytes(command, typeof(TCommand), _jsonSerializerOptions); | |
var data = JsonSerializer.SerializeToUtf8Bytes(command, _jsonSerializerOptions); |
|
||
_jsonSerializerContext = new BiDiJsonSerializerContext(jsonSerializerOptions); | ||
// Add base BiDi generated context resolver; keep options mutable for module contexts | ||
_jsonSerializerOptions.TypeInfoResolverChain.Add(BiDiJsonSerializerContext.Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should factor out BiDiJsonSerializerContext
to instead use the Initialize
method instead
Nice! Thank you. Parameterless ctor for Module looks ugly. But we need something parameterless to be able to |
User description
Contributes to #15329
🔗 Related Issues
💥 What does this PR do?
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes
PR Type
Enhancement
Description
Refactor BiDi module instantiation to use factory pattern
Add module initialization extension point for JSON serialization
Replace direct constructors with
Module.Create<T>()
methodImplement JSON serializer context configuration in modules
Diagram Walkthrough
File Walkthrough
13 files
Replace direct module constructors with factory method
Add JSON serializer context and initialization method
Add empty initialization method implementation
Add JSON context configuration and refactor serialization
Add NotImplementedException initialization method
Add NotImplementedException initialization method
Add NotImplementedException initialization method
Implement factory pattern with initialization hooks
Add NotImplementedException initialization method
Add NotImplementedException initialization method
Add NotImplementedException initialization method
Add NotImplementedException initialization method
Add NotImplementedException initialization method